Skip to content

[DEV-12952] Add Configurable Retry Behavior #350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[DEV-12952] Add Configurable Retry Behavior #350

wants to merge 2 commits into from

Conversation

mawelborn
Copy link
Contributor

@mawelborn mawelborn commented Jan 29, 2025 •
edited
Loading

Summary

This PR implements configurable, best-practices retry behavior for both IndicoClient and AsyncIndicoClient with consistent socket connect and read timeouts. This replaces the existing retry behavior that's only present on the async client and isn't configurable. IndicoConfig has new options to control retry behavior, with reasonable defaults:

IndicoConfig(
    retry_count: int = 4,  # Retry API calls this many times.
    retry_wait: float = 1,  # Wait this many seconds after the first error before retrying.
    retry_backoff: float = 4,  # Multiply the wait time by this amount for each additional error.
    retry_jitter: float = 1,  # Add a random amount of time (up to this percent as a decimal) to the wait time to prevent simultaneous retries.
)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • Jira story/task is put into PR title
  • Code has been self-reviewed
  • Hard-to-understand areas have been commented
  • Documentation has been updated
  • My changes generate no new warnings
  • Unit tests have been added for base functionality and known edge cases
  • Any dependent changes have been merged and published in downstream modules

@mawelborn mawelborn added the enhancement New feature or request label Jan 29, 2025
@mawelborn mawelborn self-assigned this Jan 29, 2025
@mawelborn mawelborn requested a review from a team as a code owner January 29, 2025 16:38
@mawelborn mawelborn force-pushed the mawelborn/configurable-retry branch 6 times, most recently from f68f18f to 3a7f846 Compare October 16, 2025 21:03
@mawelborn
Add configurable retry behavior with reasonable defaults for count, w…
32b1a8c 
…ait, backoff, and jitter

This makes retry behavior consistent between sync and async clients, and
in its default configuration will get users past network interruptions.
@mawelborn
Make socket connect and read timeouts consistent between sync and asy…
1f9ebc1 
…nc clients
@mawelborn mawelborn force-pushed the mawelborn/configurable-retry branch from 3a7f846 to 1f9ebc1 Compare October 16, 2025 21:18
@mawelborn
Copy link
Contributor Author

Hi @IndicoDataSolutions/pr-be-indicodata-ai folks! I rebased this PR onto main and have all the checks passing. Would appreciate a review when you have a chance. This functionality will be super useful for ProServe and customers.

thearchitector
thearchitector reviewed Oct 16, 2025
View reviewed changes
Copy link
Contributor

@thearchitector thearchitector left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is an urgent thing, this can probably wait, but on the platform side we've been converging on this library, which might be worth considering here instead of shipping our own? it has fancy timing, logging, and backoff stuff we could probably expose if we wanted for better script integrations

indico/config/config.py
self.requests_params: "Optional[AnyDict]" = None
self._disable_cookie_domain: bool = False

self.retry_count: int = int(os.getenv("INDICO_retry_count", "4"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

caps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thearchitector thearchitector thearchitector left review comments

@goatrocks goatrocks Awaiting requested review from goatrocks goatrocks is a code owner automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai

@sihrc sihrc Awaiting requested review from sihrc sihrc is a code owner automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai

@Sung96kim Sung96kim Awaiting requested review from Sung96kim Sung96kim is a code owner automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai

@jacobmanderson jacobmanderson Awaiting requested review from jacobmanderson jacobmanderson is a code owner automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai

@nicholas-lockhart nicholas-lockhart Awaiting requested review from nicholas-lockhart nicholas-lockhart is a code owner automatically assigned from IndicoDataSolutions/pr-be-indicodata-ai

At least 1 approving review is required to merge this pull request.

Assignees

@mawelborn mawelborn

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mawelborn @thearchitector